Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x509 Verification: Extension policies documentation. #11800

Merged
merged 12 commits into from
Feb 14, 2025

Conversation

deivse
Copy link
Contributor

@deivse deivse commented Oct 19, 2024

Finally coming back with another PR. The idea is that this one will serve to confirm the user-facing API for custom extension policy support, and the next one will have the first parts of the implementation.

@deivse deivse changed the title Add documentation related to custom x509 verification extension policies. x509 Verification: Extension policies documentation. Oct 19, 2024
@deivse deivse force-pushed the x509_verification_extension_policy_builder_docs branch from d7c5ebd to 649dd50 Compare October 19, 2024 10:00
Comment on lines 341 to 357
.. staticmethod:: webpki_defaults_ca()

Creates an ExtensionPolicyBuilder initialized with a
CA extension policy based on CA/B Forum guidelines.

This is the CA extension policy used by :class:`PolicyBuilder`.

:returns: An instance of :class:`ExtensionPolicyBuilder`

.. staticmethod:: webpki_defaults_ee()

Creates an ExtensionPolicyBuilder initialized with an
EE extension policy based on CA/B Forum guidelines.

This is the EE extension policy used by :class:`PolicyBuilder`.

:returns: An instance of :class:`ExtensionPolicyBuilder`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woodruffw right now our extension policies are basically "CABF, but with some things loosened where necessary in practice", is there a good way of articulating that for users?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@woodruffw right now our extension policies are basically "CABF, but with some things loosened where necessary in practice", is there a good way of articulating that for users?

Sorry, missed this ping originally!

Maybe something like "these extension policies are intended to be strictly consistent with CABF, which dictates the requirements for certificates on the public web. However, we make limited exceptions to CABF where necessary in practice, i.e. behaviors that CABF forbids but are nonetheless present in the current Web PKI."

But maybe that's too much of a mouthful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can put it in a glossary entry or something

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not quite sure how to formulate this as a glossary entry. What would be the entry name? I would appreciate if someone could write out the full text of the entry here (or just made a separate PR for this).

@deivse
Copy link
Contributor Author

deivse commented Oct 25, 2024

Hey, @alex, friendly ping 😄

@alex
Copy link
Member

alex commented Oct 25, 2024

Not forgotten! On my TODO list, hopefully this evening.

Thanks so much for your patience, and sorry I've been so slow. Don't feel bad about pinging me.

@deivse
Copy link
Contributor Author

deivse commented Oct 25, 2024

No worries at all, my plate is also very full right now, so I wouldn't have gotten to doing any actual changes before this weekend at the earliest anyways.

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few tiny notes, but overall I think this design works.

Not going to merge until we have an implementation of course.

@deivse
Copy link
Contributor Author

deivse commented Dec 9, 2024

Hi, sorry for disappearing for so long - other responsibilities caught up to me and I couldn't find the time unfortunately. I'll start preparing the main PR for primetime now, hopefully I'll be able to open it this year 😅

@deivse deivse force-pushed the x509_verification_extension_policy_builder_docs branch from 0639af6 to bf4412d Compare December 9, 2024 08:46
@deivse
Copy link
Contributor Author

deivse commented Dec 9, 2024

*rebased on main to keep my local branches in sync and based on main.

@deivse deivse force-pushed the x509_verification_extension_policy_builder_docs branch from bf4412d to 8039cb2 Compare January 29, 2025 14:25
@deivse
Copy link
Contributor Author

deivse commented Jan 29, 2025

Another rebase on main. I'm going to finally create the main PR now.

@alex
Copy link
Member

alex commented Jan 29, 2025 via email

@deivse deivse marked this pull request as draft February 11, 2025 10:49
@deivse
Copy link
Contributor Author

deivse commented Feb 11, 2025

Converting to draft since this will require edits after the implementation gets into main.

@deivse deivse force-pushed the x509_verification_extension_policy_builder_docs branch from 8039cb2 to 627dc16 Compare February 13, 2025 08:59
@deivse deivse force-pushed the x509_verification_extension_policy_builder_docs branch from 627dc16 to 1f6c27c Compare February 13, 2025 09:00
@@ -181,6 +179,11 @@ the root of trust:

.. versionadded:: 42.0.0

.. versionchanged:: 45.0.0
``subject``, ``verification_time`` and ``max_chain_depth`` were replaced by the
``policy`` property that provides access to these values.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an established way how deprecated properties are treated in the docs? I did a quick search and couldn't find anything like a deprecated flag in sphinx Should the properties be kept in the docs and somehow marked as deprecated or removed altogether?

Copy link
Contributor Author

@deivse deivse Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the deprecated sphinx attribute, but it doesn't seem to be intended for use on a method level. For now I changed the text in the versionchanged description to reflect that the properties were deprecated, but didn't add them back as separate attributes. I think that strikes a good balance between acknowledging the change and not cluttering the docs with deprecated functionality.

@deivse deivse marked this pull request as ready for review February 13, 2025 10:05
@deivse deivse requested a review from alex February 13, 2025 10:06
@alex
Copy link
Member

alex commented Feb 13, 2025

Not about the docs, but I just realized:

Using custom extension policies + a server verifier will result in the SAN not actually being checked, right? I think we need to fix that.

@deivse
Copy link
Contributor Author

deivse commented Feb 13, 2025

Yes, the current implementation allows to create a ServerVerifier that doesn't check SANs match if you explicitly replace the validator.

The default validator for SubjectAlternativeName does two things - checks criticality:

match (cert.certificate().subject().is_empty(), extn.critical) {
    // If the subject is empty, the SAN MUST be critical.
    (true, false) => {
        return Err(ValidationError::new(ValidationErrorKind::Other(
            "EE subjectAltName MUST be critical when subject is empty".to_string(),
        )));
    }
    // If the subject is non-empty, the SAN MUST NOT be critical.
    (false, true) => {
        return Err(ValidationError::new(ValidationErrorKind::Other(
            "EE subjectAltName MUST NOT be critical when subject is nonempty".to_string(),
        )))
    }
    _ => (),
};

and compares subject to the value in the extension:

// NOTE: We only verify the SAN against the policy's subject if the
// policy actually contains one. This enables both client and server
// profiles to use this validator, **with the expectation** that
// server profile construction requires a subject to be present.
if let Some(sub) = policy.subject.as_ref() {
    let san: SubjectAlternativeName<'_> = extn.value()?;
    if !sub.matches(&san) {
        return Err(ValidationError::new(ValidationErrorKind::Other(
            "leaf certificate has no matching subjectAltName".into(),
        )));
    }
}

It does imo make sense to make the second check mandatory, but not the first one - if someone needs to handle a certificate with incorrect criticality there, I think we should let them.

So what I suggest is to always explicitly run the name comparison (second check), irrespective of what the extension policy says. If the ExtensionPolicy does have a SAN validator, we will run both that and the name match check.

However we still have to prohibit the extension policy used with a server verifier from permitting certificates without the SAN extension. Unfortunately the "earliest" place where this can be done is in build_server_verifier since before that we don't know what the ExtensionPolicy will apply to. So I think this should be part of validation performed in cryptography_x509_verification::Policy::new, which will check that the ExtensionPolicy requires the SAN extension to be present whenever Policy.subject is not None.

@deivse
Copy link
Contributor Author

deivse commented Feb 13, 2025

To summarize, what I suggest is always checking the name in a ServerVerifier, irrespective of the ExtensionPolicy's validator, and build_server_verifier raising an exception if the ExtensionPolicy has anything except must_be_set for the SAN extension. If this sounds good, I can make a PR tomorrow.

@alex
Copy link
Member

alex commented Feb 13, 2025 via email

@alex
Copy link
Member

alex commented Feb 13, 2025 via email

@deivse
Copy link
Contributor Author

deivse commented Feb 13, 2025

Yes, the second part isn't needed, I was just slightly prioritizing the user experience here - as a user I would prefer to get an exception the moment I provide an incorrect argument to the library (extension policy that doesn't require SAN passed to build_server_verifier), instead of it happening later when I call another function (ServerVerifier.verify). It's up to you ultimately, I'm not sure if the UX improvement outweighs the extra complexity here.

@alex
Copy link
Member

alex commented Feb 13, 2025 via email

@deivse
Copy link
Contributor Author

deivse commented Feb 13, 2025

Ok, I'll implement it tomorrow and see if it makes sense to include the check in cryptography_x509_verification::Policy::new or not.

@alex
Copy link
Member

alex commented Feb 13, 2025 via email

:type: int

.. type:: MaybeExtensionValidatorCallback
:canonical: Callable[[Policy, Certificate, Optional[ExtensionType]], None]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is this the right way to document the generic parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also like to know 😄 I was trying to find documentation on how this is supposed to be done, but I didn't find anything specific to type aliases. I did try a couple things just now:

  1. Version A

    .. type:: MaybeExtensionValidatorCallback[T: ExtensionType]
        :canonical: Callable[[Policy, Certificate, Optional[T]], None]

    results in
    Screenshot 2025-02-14 at 8 44 20
    which is a bit too cluttered imo, and also triggers a warning verification.rst:445: WARNING: py:class reference target not found: T [ref.class]

  2. Version B

    .. type:: MaybeExtensionValidatorCallback
        :canonical: Callable[[Policy, Certificate, Optional[T:ExtensionType]], None]

    This is less cluttered, but doesn't show that this is a generic type as well, and still triggers a warning.
    Screenshot 2025-02-14 at 8 47 06

I don't really know if there is a proper way to do this without triggering warnings. I couldn't find anything in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are also methods on ExtensionPolicy which are generic, but I didn't document as such in this PR. After doing some quick research, I think the way this is intended to be used might be incompatible with how the cryptography docs are structured. The sphinx docs suggest here that generic functions/methods should be documented as such:

.. py:function:: add[T](a: T, b: T) -> T

but cryptography docs don't put types in the declaration there, just the names (from what I saw at least). And using a generic "defined" in the function declaration part of the doc in the argument documentation once again results in warnings and looks too cluttered imo anyway:

    .. method:: may_be_present[T: ExtensionType](extension_type, criticality, validator_cb)

        Specifies that the extension...

        :param type[T] extension_type: A concrete class derived from :type:`~cryptography.x509.ExtensionType`
            indicating which extension may be present.
        :param Criticality criticality: The criticality of the extension
        :param validator_cb: An optional Python callback to validate the extension value. 
            Must accept extensions of type `extension_type`.
        :type validator_cb: :type:`MaybeExtensionValidatorCallback[T]` or None

Warnings:

verification.rst:2: WARNING: py:class reference target not found: T [ref.class]
verification.rst:367: WARNING: py:type reference target not found: MaybeExtensionValidatorCallback[T] [ref.type]

Result:
Screenshot 2025-02-14 at 8 58 41

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I think I might prefer to leave the docs as is even if there is a proper way to document this without the warnings - I think the API is understandable without this in the docs, and it should be freely and conveniently available for anyone actually using the library with a type checker, while adding this might actually make the docs somewhat harder to parse visually and make things seem more complex than they are from a user perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: Even if this is not the right way to document this, writing ExtensionType does communicate that any T: ExtensionType will be accepted. The only thing we lose is the information that the type alias is generic, which I'm not sure is that important to the user.

@deivse
Copy link
Contributor Author

deivse commented Feb 14, 2025

Just realized there are still a couple important things missing from the docs:

Sorry, something went wrong.

@alex alex merged commit e8bf6a1 into pyca:main Feb 14, 2025
63 checks passed
@alex
Copy link
Member

alex commented Feb 14, 2025

I think the only thing remaining is a CHANGELOG entry!

@deivse
Copy link
Contributor Author

deivse commented Feb 14, 2025

Well I was just editing the docs so it mentions that an EE extension policy must require SAN to be present if it's used to build a ServerVerifier. Do you think that it's not worth mentioning as it's implied and the exception is self-explanatory, or should I make another PR?

@alex
Copy link
Member

alex commented Feb 14, 2025 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants